[Fix] improve liqoctl uninstall error message #2825
Conversation
this patch improves the liqoctl uninstall error message when the pre-uninstall checks fail, by suggesting the user which command has to be used to unpeer a cluster.
|
Hi @claudiolor. Thanks for your PR! I am @adamjensenbot.
Make sure this PR appears in the liqo changelog, adding one of the following labels:
|
| @@ -66,7 +66,10 @@ func (o *Options) Run(ctx context.Context) error { | |||
|
|
|||
| s := o.Printer.StartSpinner("Running pre-uninstall checks") | |||
| if err := utils.PreUninstall(ctx, o.CRClient); err != nil { | |||
| s.Fail("Pre-uninstall checks failed: ", output.PrettyErr(err)) | |||
| errMsg := fmt.Sprintf("Pre-uninstall checks failed: %s\n"+ | |||
There was a problem hiding this comment.
Are we checking here also for offloaded namespaces? if so, I would add the command to unoffload namespaces too
| @@ -66,7 +66,10 @@ func (o *Options) Run(ctx context.Context) error { | |||
|
|
|||
| s := o.Printer.StartSpinner("Running pre-uninstall checks") | |||
| if err := utils.PreUninstall(ctx, o.CRClient); err != nil { | |||
| s.Fail("Pre-uninstall checks failed: ", output.PrettyErr(err)) | |||
| errMsg := fmt.Sprintf("Pre-uninstall checks failed: %s\n"+ | |||
| "You can disable the active peerings with other clusters with the 'liqoctl unpeer' command.\n"+ | |||
There was a problem hiding this comment.
I would say "You must tear down the active peerings..." instead of "You can disable the active peering...."
Saying "you can", it means (to me) that it's just a suggestion, not a mandatory requirement to proceed.
There was a problem hiding this comment.
The issue here is that the errors might not be related to the active peerings, but as @aleoli said, there might be also the possibility that there still is an offloaded namespace or other resources to remove. So I wouldn't always print to "You must tear down the active peerings..." because this is not always the case.
We might add at the end of the error message something like:
- To disable active peering launch liqoctl X
- To unoffload a namespace launch liqoctl Y
Actually, the ideal solution would be changing the returned error message to suggest a solution for each of the returned error, but since this is a shared function, I wouldn't suggest to use liqoctl when the user is not actually using it. So we should add a logic to understand when we are using liqoctl and print the suggestion. However, I'm not sure if it is worth it.
What do you guys think? @aleoli @fra98 @cheina97
Maybe in future, it would be nice having a flag like --force that automatically removes everything that prevents Liqo from being uninstalled.
Description
this patch improves the liqoctl uninstall error message when the pre-uninstall checks fail, by suggesting the user which command has to be used to unpeer a cluster.
Closes #2821